Skip to content

cc-wrapper: add glibcxxassertions hardening flag#414987

Merged
risicle merged 2 commits intoNixOS:stagingfrom
risicle:ris-glibcxx-assertions
Aug 29, 2025
Merged

cc-wrapper: add glibcxxassertions hardening flag#414987
risicle merged 2 commits intoNixOS:stagingfrom
risicle:ris-glibcxx-assertions

Conversation

@risicle
Copy link
Contributor

@risicle risicle commented Jun 8, 2025 •
edited
Loading

This adds another flag included in the 2025 openssf hardening guide: https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++.html#precondition-checks-for-c-standard-library-calls

As ever, as a new hardening flag I have disabled it by default except in pkgsExtraHardening. Because of the possibility of slowdowns, this is one of the flags I probably won't advocate moving to default-enabled any time soon.

This hardening flag is a little bit odd because it only has an effect when compiling against glibc's libstdc++, but note how I haven't included any mechanism to "unsupport" it on platforms that use libc++. This is because we don't currently have any nice mechanism to detect what c++ standard library a platform is using - and whether we're using clang or gcc is a poor approximation of this. Instead this PR will happily add the -D_GLIBCXX_ASSERTIONS definition on all platforms, whether using libstdc++ or not, with the idea that other c++ standard libraries will just ignore it.

Clang's libc++ does have a similar feature that I'm planning on introducing in a separate PR. Why not put both flags under a common generic hardening flag name? Firstly I'm not convinced that the assertions of each library will have similar enough package-breaking characteristics, and secondly libc++'s hardening feature has multiple levels that I think are worth exposing separately. Once we get into multiple levels of strictness, ensuring sensible parity between the exposed levels between the two implementations feels like it would be a fool's errand.

I built quite a lot of c++ packages on aarch64-linux with this default-enabled and only found arrow-cpp's build to be affected by it.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • Nixpkgs 25.11 Release Notes (or backporting 24.11 and 25.05 Nixpkgs Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
  • NixOS 25.11 Release Notes (or backporting 24.11 and 25.05 NixOS Release notes)
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@risicle risicle requested review from a team June 8, 2025 11:20
@github-actions github-actions bot added 6.topic: stdenv Standard environment 8.has: documentation This PR adds or changes documentation labels Jun 8, 2025
@github-actions github-actions bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Jun 8, 2025
@nix-owners nix-owners bot requested review from cpcloud, tobim and veprbl June 8, 2025 11:29
@reckenrode
Copy link
Contributor

reckenrode commented Jun 8, 2025 •
edited
Loading

This is because we don't currently have any nice mechanism to detect what c++ standard library a platform is using - and whether we're using clang or gcc is a poor approximation of this.

Can’t we check for libc++ by checking for libcxx being non-null in the wrapper?

Considering that it’s possible to use libc++ on Linux (which uses libstdc++ by default) and libstdc++ on Darwin (which uses libc++), the place to check would be in the wrapper (assuming it is desired to check and toggle these assertions on our off based on the target C++ stdlib).

@risicle
Copy link
Contributor Author

risicle commented Jun 8, 2025 •
edited
Loading

So.. we probably could, but that would involve putting the logic into cc-wrapper's default.nix, and I've been trying to move logic out of cc-wrapper into the compiler derivation passthrus. And that's what I'd consider a "nice" way of doing it - having the c++ library derivation self-report unsupported flags. libstdc++ doesn't even have an identifiable package to annotate this onto.

@reckenrode
Copy link
Contributor

reckenrode commented Jun 8, 2025

libstdc++ doesn't even have an identifiable to annotate this onto.

So maybe putting it there if/when GCC gets split into separate packages?

risicle added 2 commits August 24, 2025 11:56
@risicle
cc-wrapper: add glibcxxassertions hardening flag
3eec07e 
no platforms "unsupported" because we don't have a nice
mechanism for determining a platform's c++ lib and the flag
should be harmlessly ignored by a other c++ libs
@risicle
arrow-cpp: disable glibcxxassertions hardening flag
1293be8
@risicle risicle force-pushed the ris-glibcxx-assertions branch from 2529a05 to 1293be8 Compare August 24, 2025 19:36
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. labels Aug 24, 2025
@philiptaron
Copy link
Contributor

philiptaron commented Aug 25, 2025

What's new in these changes Robert @risicle?

Note that GCC in multiple packages landed

@risicle
Copy link
Contributor Author

risicle commented Aug 25, 2025

It looks like gccNGPackages hasn't yet implemented hardeningUnsupportedFlags or hardeningUnsupportedFlagsByTargetPlatform - I should probably tell someone. It looks like a step in the right direction, but I don't think this will be able to perform c++ lib detection until we decide upon a standard stdenv attr for it a la stdenv.cc.

Apart from a straight rebase, this PR hasn't changed since posting.

@philiptaron
Copy link
Contributor

philiptaron commented Aug 25, 2025

Someone would be John @Ericson2314 or Tristan @RossComputerGuy for gccNGPackages, I believe.

philiptaron
philiptaron approved these changes Aug 27, 2025
View reviewed changes
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with trusting @risicle on this. Randy do you object enough to close the PR until another way is found?

@github-project-automation github-project-automation bot moved this to In Progress in Stdenv Aug 27, 2025
@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Aug 27, 2025
@risicle risicle merged commit d3afbb6 into NixOS:staging Aug 29, 2025
32 of 34 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Stdenv Aug 29, 2025
@risicle risicle mentioned this pull request Aug 29, 2025
Merged
13 tasks
@risicle risicle mentioned this pull request Sep 14, 2025
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@philiptaron philiptaron philiptaron approved these changes

@hsjobeki hsjobeki Awaiting requested review from hsjobeki

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@emilazy emilazy Awaiting requested review from emilazy

@GetPsyched GetPsyched Awaiting requested review from GetPsyched

@reckenrode reckenrode Awaiting requested review from reckenrode

@fricklerhandwerk fricklerhandwerk Awaiting requested review from fricklerhandwerk

@Artturin Artturin Awaiting requested review from Artturin

@RossComputerGuy RossComputerGuy Awaiting requested review from RossComputerGuy

@tobim tobim Awaiting requested review from tobim

@veprbl veprbl Awaiting requested review from veprbl

@cpcloud cpcloud Awaiting requested review from cpcloud

Assignees

No one assigned

Labels

6.topic: stdenv Standard environment 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

Stdenv
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@risicle @reckenrode @philiptaron